Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix find_program ~ don't report errors for failed simple checks #247

Closed
wants to merge 1 commit into from

Conversation

rivy
Copy link
Contributor

@rivy rivy commented Nov 2, 2018

When searching for tools, errors were emitted from the "check" function when a tool was missing from a searched path. That is expected behavior and I would say should have only an informational note or nothing at all.

This PR moves any check with a non-function opt.check together with the check done with a missing opt.check. That check never emits an error, so this change suppresses the, IMO unnecessary, error noted above.

@waruqi
Copy link
Member

waruqi commented Nov 2, 2018

This pr is unnecessary, I need to add -v argument to run xmake -v for knowing why the detection tool failed. That is expected behavior.

And if you do not add the -v parameter, this error message has been already suppressed.

@rivy
Copy link
Contributor Author

rivy commented Nov 2, 2018

Ok, I understand that you might want to see information if the detection tool failed. But, to me, an "error" denotes something that is configured incorrectly and needs to be fixed. It's concerning when I see an "error" or "warning" during builds, even if only during a verbose (-v) build. And I think lots of other devs would see the "error" and spend time searching for a repair as well.

From my reading of the source code, the check function can be run several times while searching through paths for a tool and will, during normal operation, "fail" for earlier paths which don't contain the tool until finding it in a later path (driven by the find_program() function). I initially thought about adding a "warning" and/or "info" function, eg...

-- the verbose warning function
function utils.vwarning(format, ...)

    -- enable verbose?
    if option.get("verbose") and format ~= nil then
        utils.warning(format, ...)
    end
end

-- the verbose information function
function utils.vinfo(format, ...)

    -- enable verbose?
    if option.get("verbose") and format ~= nil then
        -- ... bright blue info message ...
    end
end

... and then changing check to issue a vwarning or vinfo instead of verror. But I thought it better to move the simple check up into the context of the initial simple check when I saw that the initial exec can fail without issuing any error:

https://github.com/tboox/xmake/blob/d130b8049af3250f3504580d146e3c54ed052a5f/xmake/core/sandbox/modules/import/lib/detect/find_program.lua#L57-L60

It seems to me that given what you want (and I agree that knowing which paths failed for a tool search is useful), check should issue an informational note if it doesn't find the tool on a specific path (including in the above notes section of code), and maybe find_program() or some higher level function should issue an error if the tool is truly, finally, not found in all searched locations.

@waruqi
Copy link
Member

waruqi commented Nov 3, 2018

Ok, I understand and agree with you, as the error message is really not friendly during a verbose (-v) build. But I haven’t thought about how to improve the display detection failure information. Maybe utils.vinfo, add xmake -v --more or xmake -V arguments to get more verbose info only for developers, and etc.

I need to take some time to think about it.

@rivy rivy changed the base branch from master to dev November 3, 2018 03:35
@waruqi
Copy link
Member

waruqi commented Nov 4, 2018

I add -D and --diagnosis arguments to get more diagnosis information only for developers.
d3d05b0

$ xmake -h
  -v, --verbose      Print lots of verbose information for users.
  -D, --diagnosis    Print lots of diagnosis information (backtrace, check info ..) only for developers.
                     And we can append -v to get more whole information.
                           e.g. $ xmake -v -D

And I have improved the check errors output:

    if not ok and option.get("diagnosis") then
        utils.cprint("${yellow}checkinfo: ${clear dim}" .. errors)
    end

And the premise of using this option (-D/--diagnosis) is that the developer has already understood the meaning of all error message output, for example:
Suppose the developer knows whether the error can be ignored or must be fixed.

@waruqi waruqi closed this Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants